Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

defer calls to ScanEvents & friends if it doesn't come from WeakAuras #5555

Merged
merged 2 commits into from
Nov 30, 2024

Conversation

emptyrivers
Copy link
Contributor

Primarily this ensures that we don't have any kind of reentrancy, which has the potential to break invariants.

@emptyrivers emptyrivers added the 🏨 Bugfix This pull request fixes an issue. label Nov 25, 2024
@emptyrivers
Copy link
Contributor Author

I believe that with this change, there's also some machinery in AuraEnvironment which is unnecessary (because its purpose is to defend against exactly the reentrancy which is now impossible)

Primarily this ensures that we don't have any kind of reentrancy,
which has the potential to break invariants.

Fixes WeakAuras#5543
@InfusOnWoW
Copy link
Contributor

The code looks good, though I haven't tested it against the ticket to check if it solves that problem.

I have two potential concerns:
a) Some external addons call WeakAuras.ScanEvents(). Should that affect how we do this.
b) A potential other solution would be to have the same "deferred queue", but that the handling of that queue is done at the end of all code paths that can lead to the queue getting filled.

The issue in the linked ticket was iirc from an every frame update of an text. Having the deferred handling via a separate frame means that the ScanEvents potentially happens at the next frame, depending on how wow iterates the frames for their OnUpdate handler. I would suspect that some people rely on the same frame handling that is currently always the case.

@emptyrivers
Copy link
Contributor Author

a) Some external addons call WeakAuras.ScanEvents(). Should that affect how we do this.

I did think about that. I suspect in most cases those addons are using WeakAuras as a "frontend" display and aren't expecting any kind of backflow of data or control. For those cases the point is moot, and for the rest, we could commit to the async bit and allow callers to register a post-callback once we process the event. But that seems niche enough that i'd like to see a real use case before implementing it.

@emptyrivers
Copy link
Contributor Author

b) A potential other solution would be to have the same "deferred queue", but that the handling of that queue is done at the end of all code paths that can lead to the queue getting filled.

That would work but it will lead to at least one stupid bug in the future because we forgot about deferred events when implementing something new. The benefits aren't obvious enough for me to want to point a gun at my foot, but i could be convinced.

since C_Tooltip functions complain when given nil
@InfusOnWoW
Copy link
Contributor

I think I'm fine with it as is. I'll do a quick test against the actual problematic aura, and merge it then.

@InfusOnWoW InfusOnWoW merged commit 2c67ffb into WeakAuras:main Nov 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏨 Bugfix This pull request fixes an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants